fix: removed duplicate code in KubeConfigUpdate. Added tests#310
fix: removed duplicate code in KubeConfigUpdate. Added tests#310adietish wants to merge 1 commit into
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors KubeConfigUpdate to inject a persisterFactory and centralize persistence and context-entry creation; exposes KubeConfigNamedContext.toName as ChangesKubeconfig persistence and context refactoring
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt (1)
74-75: ⚡ Quick winAvoid positional
saveConfigoverloads with identical collection types.These overloads take
users,clusters, andcontextsas separateArrayList<Any?>parameters, and the private delegate reorders them. A future call-site swap will still compile and can write a malformed kubeconfig. PassingContextEntriesdirectly would make this helper much safer.Refactor direction
- protected fun saveConfig( - config: KubeConfig, - users: ArrayList<Any?>, - clusters: ArrayList<Any?>, - contexts: ArrayList<Any?>, - currentContext: String - ) { - saveConfig(contexts, clusters, users, config.preferences, currentContext, config.path) - } + protected fun saveConfig(config: KubeConfig, entries: ContextEntries) { + saveConfig( + entries.contexts, + entries.clusters, + entries.users, + config.preferences, + entries.currentContextName, + config.path + ) + }Then update the two callers to pass
entries/contextEntriesdirectly.Also applies to: 85-95
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt` around lines 74 - 75, The saveConfig overloads currently accept separate ArrayList<Any?> for users, clusters, and contexts which is error-prone; change the private helper signature (the delegated saveConfig) to accept the strongly-typed ContextEntries (or the appropriate ContextEntries wrapper type) instead of positional ArrayList<Any?> parameters, update the two callers that currently pass entries / contextEntries to pass the ContextEntries object directly, and remove any manual reordering logic inside saveConfig so the original entries are preserved by type; reference the saveConfig overloads and the callers that pass entries/contextEntries to locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt`:
- Around line 209-214: When setting a token in setTokenFor (and in the
UpdateToken flow), also clear any existing client cert fields to avoid
presenting both credentials: after Utils.setValue(this, token,
arrayOf("user","token")) remove or null out "user.client-certificate-data" and
"user.client-key-data" on the same user object (use the existing Utils helpers —
e.g., Utils.setValue(this, null, arrayOf("user","client-certificate-data")) /
Utils.setValue(this, null, arrayOf("user","client-key-data")) or the
Utils.removeValue equivalent) so UpdateToken mirrors UpdateClientCert's behavior
of removing the opposite auth field.
In
`@src/test/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdateTest.kt`:
- Around line 29-32: The tests currently use a single mock (mockPersister)
returned by testPersisterFactory so they only assert multiple save() calls
rather than per-file routing; change testPersisterFactory to create and return a
distinct mock ConfigPersister for each File (store them in a map keyed by
File/path) and update assertions to verify each file's dedicated mock was
invoked exactly once (e.g., verify(exactly = 1) {
persistersByFile[userFile].save(...) } and verify for the other file including
the context name matcher) so the test validates that each config file is
persisted by its own factory-produced persister.
---
Nitpick comments:
In `@src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt`:
- Around line 74-75: The saveConfig overloads currently accept separate
ArrayList<Any?> for users, clusters, and contexts which is error-prone; change
the private helper signature (the delegated saveConfig) to accept the
strongly-typed ContextEntries (or the appropriate ContextEntries wrapper type)
instead of positional ArrayList<Any?> parameters, update the two callers that
currently pass entries / contextEntries to pass the ContextEntries object
directly, and remove any manual reordering logic inside saveConfig so the
original entries are preserved by type; reference the saveConfig overloads and
the callers that pass entries/contextEntries to locate the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: d0d178b7-c346-4934-b30f-3011c0683346
📒 Files selected for processing (3)
src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigEntries.ktsrc/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.ktsrc/test/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdateTest.kt
c40fe65 to
30e9536
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #310 +/- ##
==========================================
+ Coverage 0.00% 23.17% +23.17%
==========================================
Files 4 99 +95
Lines 26 4182 +4156
Branches 0 770 +770
==========================================
+ Hits 0 969 +969
- Misses 26 3089 +3063
- Partials 0 124 +124 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bdb5da6 to
65b246a
Compare
|
@vrubezhny, @azatsarynnyy: please review |
65b246a to
d6453ac
Compare
Signed-off-by: Andre Dietisheim <adietish@redhat.com> Assisted-by: Cursor <cursoragent@cursor.com>
d6453ac to
2126dbb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/kotlin/com/redhat/devtools/gateway/openshift/OpenShiftClientFactory.kt`:
- Around line 281-290: createUser currently inconsistently detects token auth
(checks token != null elsewhere but here also requires token.isNotEmpty()),
which allows an empty token and absent certs to produce a user with no auth; fix
by making the branch consistent: either (A) treat any non-null token as token
auth by removing token.isNotEmpty() so setToken(token, user) is called when
token != null, or (B) explicitly validate and fail fast—if token != null but
token.isEmpty() and clientCert/clientKey are null, throw an
IllegalArgumentException (or log and throw) instead of falling through to
setClientCertificates; update the logic in createUser and keep using the
existing helper functions setToken and setClientCertificates to populate the
user map.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: f2acc3c6-103b-4dba-bbb4-6720e6c871c9
📒 Files selected for processing (5)
src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigEntries.ktsrc/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.ktsrc/main/kotlin/com/redhat/devtools/gateway/openshift/OpenShiftClientFactory.ktsrc/test/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigTestHelpers.ktsrc/test/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdateTest.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigEntries.kt
- src/test/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdateTest.kt
- src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt
| private fun createUser(usingToken: Boolean, token: CharArray?, clientCert: CertificateSource?, clientKey: CertificateSource?): Map<String, Any> { | ||
| val userAuth = mutableMapOf<String, Any>() | ||
| val user = mutableMapOf<String, Any>() | ||
|
|
||
| if (usingToken | ||
| && token != null | ||
| && token.isNotEmpty()) { | ||
| userAuth["token"] = String(token).trim() | ||
| setToken(token, user) | ||
| } else { | ||
| clientCert?.let { cert -> | ||
| if (cert.isFilePath) { | ||
| userAuth["client-certificate"] = cert.value.trim() | ||
| } else { | ||
| userAuth["client-certificate-data"] = PemUtils.toBase64(cert.value.trim()) | ||
| } | ||
| } | ||
| clientKey?.let { key -> | ||
| if (key.isFilePath) { | ||
| userAuth["client-key"] = key.value.trim() | ||
| } else { | ||
| userAuth["client-key-data"] = PemUtils.toBase64(key.value.trim()) | ||
| } | ||
| } | ||
| setClientCertificates(clientCert, clientKey, user) | ||
| } |
There was a problem hiding this comment.
Fix inconsistent token-auth detection to avoid empty-auth kubeconfig users.
Line 235 treats any non-null token as token auth, but Line 285 additionally requires token.isNotEmpty(). With an empty token and no certs, this path writes no auth fields at all, producing an invalid user entry instead of failing fast.
Proposed fix
private fun createKubeConfig(
@@
- val usingToken = token != null
+ val usingToken = token?.isNotEmpty() == true
val usingClientCert = clientCert != null && clientKey != null
require(usingToken.xor(usingClientCert)) {
"Provide either token OR clientCert + clientKey."
}
@@
- private fun createUser(usingToken: Boolean, token: CharArray?, clientCert: CertificateSource?, clientKey: CertificateSource?): Map<String, Any> {
+ private fun createUser(usingToken: Boolean, token: CharArray?, clientCert: CertificateSource?, clientKey: CertificateSource?): Map<String, Any> {
val user = mutableMapOf<String, Any>()
- if (usingToken
- && token != null
- && token.isNotEmpty()) {
+ if (usingToken && token != null) {
setToken(token, user)
} else {
setClientCertificates(clientCert, clientKey, user)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/kotlin/com/redhat/devtools/gateway/openshift/OpenShiftClientFactory.kt`
around lines 281 - 290, createUser currently inconsistently detects token auth
(checks token != null elsewhere but here also requires token.isNotEmpty()),
which allows an empty token and absent certs to produce a user with no auth; fix
by making the branch consistent: either (A) treat any non-null token as token
auth by removing token.isNotEmpty() so setToken(token, user) is called when
token != null, or (B) explicitly validate and fail fast—if token != null but
token.isEmpty() and clientCert/clientKey are null, throw an
IllegalArgumentException (or log and throw) instead of falling through to
setClientCertificates; update the logic in createUser and keep using the
existing helper functions setToken and setClientCertificates to populate the
user map.
No description provided.